Skip to content

Conversation

jferrant
Copy link
Contributor

@jferrant jferrant commented Oct 9, 2025

As part of AAC, determined the desired code path to test should be accept_block (which is called in the p2p_broadcast function for miners, and in process_new_nakamoto_block_ext in the Relayer) and that these seemed to be the only points of entry to the staging blocks db. This PR just makes it clear that this is actually the case by making all other writing function calls private in nakamoto block staging db.

Confirmed that calling store_block_if_better on a shadow block has the same effect as just doing store_block. Therefore, we can use it as the single point of entry. I also updated the NakamotoChainState::accept_block fn sig to be a bit more clear (at least to me), but I can revert that if the old fn sig is preferred.

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and looks a lot safer too. Thanks!

Copy link
Contributor

@fdefelici fdefelici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this PR. It clarifies a lot of aspects around the block storage logic and introduces a clear single entry point for it. Great work on that part!

That said, I have some concerns about moving more business logic into the database layer.

In my view, the DB layer should primarily expose CRUD operations and enforce data integrity (for example, the methods made private in this PR fit well within that responsibility). However, with this change, the DB layer seems to have gained some additional business responsibilities.

This also has implications:

  • Maybe less clarity on which layer we should write business logic.
  • NakamotoChainState tests now depend (explicitly) on the DB layer’s business logic to run correctly.
  • Testing the DB itself becomes a bit more trickier (considering our test organization), since some CRUD methods are now private, we can only test DB behavior indirectly through its business methods (or integration tests). Of course, we could still work around this by adding tests in the same module or child module, requiring different test organization.

To be clear, I’m not suggesting we block this PR. I think the direction is overall positive. I just wanted to open a discussion on this point, since we have similar situations in other DB implementations, and it might be worth aligning on a common approach.

@jferrant
Copy link
Contributor Author

I really like this PR. It clarifies a lot of aspects around the block storage logic and introduces a clear single entry point for it. Great work on that part!

That said, I have some concerns about moving more business logic into the database layer.

I had the same thought while writing this PR. I hink its better to make the DB operations independent of chainstate logic, but was having a hard time enforcing the privacy restrictions i wanted as a result/I wanted to quickly prove that accept_block was truly the only point of insertion into the db. I do think its worth revisiting/additional effort to enforce better separation while keeping tighter restrictions on the db access. Let me see what I can do today cause I prob didn't give it enough thought.

@jcnelson
Copy link
Member

That said, I have some concerns about moving more business logic into the database layer.

Can you elaborate more on this point? What business logic features do you believe are present in the database layer that could be moved out?

@jferrant
Copy link
Contributor Author

That said, I have some concerns about moving more business logic into the database layer.

Can you elaborate more on this point? What business logic features do you believe are present in the database layer that could be moved out?

I was thinking specifically store_block_if_better (which mostly calls already existing db functions though). However it does have logic around orphaned blocks, signing weight, etc. that seem a bit like they shouldn't be in the db transaction? But not sure how else to set this up. I would expect the outer caller to do these sorts of checks but then it means exposing all those inner db calls...so this is why I ended up where I ended up

@jcnelson
Copy link
Member

jcnelson commented Oct 10, 2025

I was thinking specifically store_block_if_better (which mostly calls already existing db functions though). However it does have logic around orphaned blocks, signing weight, etc. that seem a bit like they shouldn't be in the db transaction? But not sure how else to set this up. I would expect the outer caller to do these sorts of checks but then it means exposing all those inner db calls...so this is why I ended up where I ended up

Right -- store_block_if_better is part of the transaction logic precisely because it has to happen within a database transaction. It's performing reads and writes on multiple records which must happen atomically with respect to other reads and writes. This atomicity requirement is what drives the current factoring. Otherwise, as you pointed out, we'd end up polluting the business logic with a lot of nitty-gritty details of transaction management, which I think is a worse outcome than the status quo since it creates more opportunities for accidental consensus bugs. To speak to @fdefelici's point, I think of this function as one of many different "update" methods exposed by the DB.

@jferrant jferrant requested a review from fdefelici October 10, 2025 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants